-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
subscriptions: kick resub ddos protection #3212
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits, but also two important things about +unsubscribe
.
@Fang- added missing leave, but unsure how to test since we don't have a way to trigger an unsubscribe from a group we don't have yet so might need your help there. |
I wouldn't be super concerned with testing that, in this case it's plainly visible from the code that we always emit a leave. But also: the dumb version of that test is testing the library directly. That is, calling the I do still notice a lack of Lastly I'd consider adding a little comment at the top of the file explaining that all subscriptions must be managed through the library. Or pointing that out at the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest commits look good. Thank you for your service!
Fixes LAND-1527 by adding a library with a mechanism for us to delay resubscriptions by 30 seconds to ensure that we don't take up all CPU/event throughput of a ship when in a kick + resubscribe loop. This was added to only groups and channels, but could stand to be applied elsewhere.
PR Checklist